-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SMOKES] Dynamic VS Vector decision + Vector Infowindows / Tooltips #1624
Conversation
…vector-decision-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🇪🇸
I did the acceptance steps and it works perfectly. One thing, vector will not work because it needs some changes that are under |
True fact @donflopez! Should we cherry-pick those changes so that we can release this? |
Yes, sure! |
I already did this! We should try it in staging. |
It is working on ded02, if you want to try @alonsogarciapablo, I think we're ready to merge :) |
Well, I just tried with a torque map and it fails with the next error:
|
At the API level, this optional setting/option is still called `vector`. It can be set to `true` to enforce vector rendering or `false` to enforce raster rendering. Any other value or not passing this option at all, will enable the auto render mode, and the library will try to use vector rendering by default.
32e2fb7
to
9db7d49
Compare
Torque is now working, it needs this changes on deep-insights.js |
I've noticed that the time series widget is not being animated like the map content. |
retest this please |
Important reminder! Some properties set by default are not supported by Tangram and the decision always goes for raster. We have to think which solution is the best for this cases: 1.- Support those properties. This can be a pain where most hurt. It is gonna take time and collaboration with Mapzen. 2.- Remove those properties from default ccss. This has some inconveniences that @makella can explain better than me, some of them are that raster needs some of them to make the visualisations smoother and we have raster in Builder. 3.- Ignore those properties in Tangram. IMO, this is the best option because we don't affect to the Builder visualisation and we can get rid of those properties that doesn't affect pretty much in visualisations terms in vector. cc/ @javisantana, @jorgesancha, @rochoa, @makella |
The one property that we have by default that isn't supported by Tangram is the On the other hand, the only place that we have this default is when polygons are first added. As soon as the user modifies any styling property, Paco and I did some testing and it seemed that the Tangram rendering did an ok job with the rendering so I'd agree with him that if we can ignore these properties and still get the map to vector rendering that would be preferable instead of something like |
I'd like to add some other properties that aren't fully supported like Thank you @makella! 🙂 |
@donflopez oh yes, that's another default cartocss property for polygons :) |
@makella thanks for your input! ❤️ So the idea is to ignore all of the default properties that are not supported by tangram? Should we make a list of these properties or they are just the ones you guys already mentioned (
Also, not sure if auto-styling might set some props that won't work with Tangram. @makella you're the boss here! Thanks! cc: @jorgesancha |
If outostyle set a not suported property we should remove it if it's not that important or add support for it. For example, in case of I understand the visual quality is not the same but we should take into account we can't do everything and we need to cut scope at some point |
We are going to release this internally as is and we can then continue with this discussion and tune things a bit more. |
…artodb.js into render-view-after-first-reload
…fter-first-reload
Retest this please! |
…artoDB/cartodb.js into render-view-after-first-reload
This PR implements 3 things:
We'll be running some SMOKE tests for these changes.
Fixes: CartoDB/cartodb#11783